Skip to content

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support - #3286

Open
sfarestam-iproov wants to merge 9 commits into
aws:mainfrom
sfarestam-iproov:add-brainpool-curves
Open

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support#3286
sfarestam-iproov wants to merge 9 commits into
aws:mainfrom
sfarestam-iproov:add-brainpool-curves

Conversation

@sfarestam-iproov

@sfarestam-iproov sfarestam-iproov commented Jun 3, 2026

Copy link
Copy Markdown

Issues:

Resolves #2939

Description of changes:

AWS-LC currently does not support Brainpool elliptic curves. This PR adds EC group support for the five Brainpool r1 prime curves defined in RFC 5639: brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, and brainpoolP512r1.

The implementation follows the same pattern as secp256k1 — generic Montgomery arithmetic via EC_GFp_mont_method(), with no hand-optimized assembly. The only structural addition is ec_group_set_a_mont() for setting an arbitrary Montgomery-form a coefficient, since Brainpool curves have a ≠ -3 (unlike NIST curves) and a ≠ 0 (unlike secp256k1). The existing ec_GFp_mont_dbl() already handles this case correctly via its a_is_minus3 == 0 code path.

NIDs (925, 927, 929, 931, 933) and OIDs were already registered in nid.h and obj_dat.h. All domain parameters are sourced from RFC 5639 Sections 3.3–3.7, with OIDs from Section 4.1.

This PR covers EC primitive support (key generation, ECDSA, ECDH). TLS negotiation support (RFC 7027 / RFC 8734) can follow in a separate PR.

Motivation: These curves are required by ICAO Doc 9303 (ePassport standard, 30+ countries), BSI TR-03116-4 (German federal regulation), and are blocking the pyca/cryptography project from accepting Brainpool improvements (pyca/cryptography#14905).

Call-outs:

  • FIPS boundary: The code is in crypto/fipsmodule/ec/. If adding curves inside the FIPS module is a concern for recertification, the implementation can be moved to crypto/ec_extra/. Please advise on the preferred placement.
  • Arbitrary a coefficient: Added a small helper ec_group_set_a_mont() (6 lines) to copy a Montgomery-form a value. This is the only new function beyond the existing curve template pattern.
  • make_tables.go changes: Added a curveWithA wrapper type and writeCurveDataWithA() function to emit MontA constants for curves where a is not -3 or 0. The standard elliptic.CurveParams in Go doesn't carry an A field (it assumes a = -3).

Testing:

  • BrainpoolKeygenSignVerify: generates a key, signs a digest with ECDSA, verifies the signature, and checks that a corrupted signature is rejected — for all 5 curves.
  • ECPKParmatersBio: round-trip i2d_ECPKParameters_bio / d2i_ECPKParameters_bio for all 5 curves.
  • GetNamedCurve: dispatches Brainpool curve names for file-based test vectors.
  • Scalar base multiplication test vectors are not included in this PR (generating them requires a Go Brainpool library); they can be added as a follow-up.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@sfarestam-iproov
sfarestam-iproov requested a review from a team as a code owner June 3, 2026 14:58
@sfarestam-iproov sfarestam-iproov mentioned this pull request Jun 3, 2026
Open
github-actions[bot]
github-actions Bot reviewed Jun 3, 2026
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread crypto/fipsmodule/ec/ec_test.cc
// Sign a digest.
std::vector<uint8_t> sig(ECDSA_size(key.get()));
unsigned sig_len;
ASSERT_TRUE(

@github-actions github-actions Bot Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'sig_len' is not initialized [cppcoreguidelines-init-variables]

crypto/fipsmodule/ec/ec_test.cc:2829:

- len;
+ len = 0;

@sfarestam-iproov sfarestam-iproov Jun 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — initialized to 0. Thanks.

@justsmth
justsmth requested review from dkostic and nebeid June 3, 2026 16:10
@codecov-commenter

codecov-commenter commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.89617% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.20%. Comparing base (e8ea407) to head (df161b2).
⚠️ Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/ec/ec_test.cc 78.72% 10 Missing ⚠️
crypto/fipsmodule/evp/evp.c 50.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3286      +/-   ##
==========================================
+ Coverage   78.18%   78.20%   +0.02%     
==========================================
  Files         693      694       +1     
  Lines      123893   124075     +182     
  Branches    17205    17215      +10     
==========================================
+ Hits        96861    97030     +169     
- Misses      26114    26127      +13     
  Partials      918      918

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nebeid

nebeid commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Thank you @sfarestam-iproov for this contribution.

  1. FIPS boundary placement — I'd lean toward moving this to crypto/ec_extra/ as you offered.

  2. Known-answer test vectors — Unless I'm missing them, I don't see any KATs. The current tests (BrainpoolKeygenSignVerify, ECPKParmatersBio) are round-trip/self-consistency checks, which would pass even if the domain parameters were subtly off as long as they're internally consistent. Since hardcoded curve parameters are the core of this change, it'd be good to validate p/a/b/G/n and the generated tables against independent reference vectors (RFC 5639, Wycheproof, or another library). I'd suggest including these in this PR rather than as a follow-up.

@sfarestam-iproov

sfarestam-iproov commented Jun 23, 2026

Copy link
Copy Markdown
Author

Thank you for the review @nebeid — both changes are now addressed:

1. FIPS boundary placement (commit 4ee8115): Moved all 5 Brainpool DEFINE_METHOD_FUNCTION blocks from crypto/fipsmodule/ec/ec.c to crypto/ec_extra/ec_brainpool.c. The small helper functions (ec_group_init_static_mont, ec_group_set_a_mont) are duplicated in the new file rather than exposed via internal.h, to avoid widening the FIPS module's internal API surface.

2. Known-answer test vectors (commit 193a2a2): Added two independent validation layers:

  • BrainpoolDomainParamsRFC5639 (ec_test.cc): Extracts p/a/b/Gx/Gy/n from each curve's EC_GROUP (converting out of Montgomery form) and compares against the hex reference values from RFC 5639 Sections 3.2–3.7. This catches subtly wrong precomputed constants that self-consistency tests would miss.

  • WycheproofECDSABrainpool (evp_test.cc): Runs Wycheproof ECDSA verify vectors for all 5 curves (~22k lines of test vectors sourced from C2SP/wycheproof), validating that the precomputed multiplication tables produce correct point arithmetic. Follows the existing WycheproofECDSAsecp256k1 pattern.

@sfarestam-iproov

sfarestam-iproov commented Jun 25, 2026

Copy link
Copy Markdown
Author

The strongswan-x86_64 CI failure is caused by our changes and is now fixed in 43d698e.

Root cause: evp_pkey_tls_encodedpoint_ec_curve_supported() in crypto/fipsmodule/evp/evp.c had a hardcoded allowlist of only four NIST curves (P-224, P-256, P-384, P-521). strongswan's openssl plugin uses EVP_PKEY_get1_tls_encodedpoint() / EVP_PKEY_set1_tls_encodedpoint() for its DH key exchange implementation. Before this PR, EC_GROUP_new_by_curve_name(NID_brainpoolP256r1) returned NULL so strongswan skipped the test vector. With Brainpool support now present, strongswan detects the curve and attempts an ECDH exchange, but the public key export/import fails at the allowlist.

Fix: Added NID_secp256k1 and all five Brainpool NIDs to the allowlist. The underlying point encoding (EC_POINT_oct2point, EC_KEY_key2buf) is curve-agnostic — the restriction was just overly conservative. The actual ECDH computation path (EVP_PKEY_derive) has no curve restrictions.

Note: the clang (Windows) failure is unrelated — it's a flaky ALPS-Decline-Server-Old-DTLS-TLS12 test failing due to Windows socket exhaustion (system lacked sufficient buffer space).

@nebeid

nebeid commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Thank you for widening the curve allowlist in evp_pkey_tls_encodedpoint_ec_curve_supported

Suggestion: add a direct unit test so this gate is guarded explicitly rather than incidentally. The existing ECTLSEncodedPoint test already does hardcoded per-curve set1/get1 round-trips for the NIST curves; extending it to cover secp256k1 and at least one Brainpool curve would pin the allowlist and prevent future drift.

@sfarestam-iproov

sfarestam-iproov commented Jun 25, 2026

Copy link
Copy Markdown
Author

Done in 18fe067 — extended the ECTLSEncodedPoint test to cover secp256k1 (Wycheproof ecdh_secp256k1_test.json tcId 1) and brainpoolP256r1 (RFC 7027 Section A.1). Both entries exercise the full set1 → EVP_PKEY_derive → get1 round-trip, pinning the allowlist.

sfarestam-iproov and others added 9 commits July 27, 2026 09:19
@sfarestam-iproov @justsmth
Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r…
209a60f 
…1, brainpoolP512r1 EC group support

Add all five Brainpool r1 prime curves from RFC 5639 using the generic
Montgomery arithmetic path (EC_GFp_mont_method), following the same
pattern as secp256k1. No hand-optimized assembly is needed.

The only structural addition beyond the secp256k1 template is
ec_group_set_a_mont() for setting an arbitrary Montgomery-form a
coefficient (Brainpool curves have a != -3 and a != 0). The existing
ec_GFp_mont_dbl() already handles this case correctly.

NIDs were already registered in nid.h. This change adds the EC_GROUP
definitions, the precomputed Montgomery constants (via make_tables.go),
the NID-to-group switch cases, and the public API functions.

Domain parameters sourced from:
 - brainpoolP224r1: RFC 5639, Section 3.3
 - brainpoolP256r1: RFC 5639, Section 3.4
 - brainpoolP320r1: RFC 5639, Section 3.5
 - brainpoolP384r1: RFC 5639, Section 3.6
 - brainpoolP512r1: RFC 5639, Section 3.7
OIDs from: RFC 5639, Section 4.1

These curves are needed for:
- ICAO 9303 ePassport certificate processing (30+ countries)
- BSI TR-03116-4 compliance (German federal regulation)
- pyca/cryptography Brainpool support (blocked on aws-lc, see aws#2939)

Resolves aws#2939 (EC primitive support; TLS negotiation can follow separately)
@sfarestam-iproov @justsmth
Fix Brainpool curve ASN.1 serialization by adding to kAllGroups
d8caac3 
The kAllGroups array in ec_asn1.c is used by EC_KEY_parse_curve_name
and EC_get_builtin_curves to look up curves by OID. Without the
Brainpool entries, i2d/d2i_ECPKParameters_bio round-trips fail with
EC_R_UNKNOWN_GROUP.
@sfarestam-iproov @claude @justsmth
Move Brainpool curve definitions outside FIPS module boundary
02b572f 
Brainpool curves are not FIPS-approved, so move their
DEFINE_METHOD_FUNCTION definitions from crypto/fipsmodule/ec/ec.c
to crypto/ec_extra/ec_brainpool.c. The helper functions
ec_group_init_static_mont and ec_group_set_a_mont are duplicated
(rather than exposed via internal.h) to avoid widening the FIPS
module's internal API surface.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Add known-answer tests for Brainpool domain parameters
f4919a6 
Add two independent validation layers:

1. BrainpoolDomainParamsRFC5639 (ec_test.cc): extracts p/a/b/G/n
   from each curve's EC_GROUP and compares against RFC 5639 reference
   hex values. This catches subtly wrong Montgomery constants in
   builtin_curves.h that self-consistency tests would miss.

2. WycheproofECDSABrainpool (evp_test.cc): runs Wycheproof ECDSA
   verify vectors for all 5 curves (~22k lines of test vectors from
   C2SP/wycheproof), validating that the precomputed tables produce
   correct point arithmetic results.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Fix FIPS build: replace DEFINE_METHOD_FUNCTION outside fipsmodule
c2b86a9 
DEFINE_METHOD_FUNCTION (from delocate.h) relies on delocated BSS
storage within bcm.o, so it cannot be used outside crypto/fipsmodule/.
Replace with an equivalent CRYPTO_once-based lazy-init macro that
works in both FIPS and non-FIPS builds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Move Brainpool NID dispatch out of FIPS module (bcm.o)
7861a2d 
Replace the 10 per-curve case arms in EC_GROUP_new_by_curve_name and
_mutable with a single default fallthrough to
ec_group_new_by_curve_name_nonfips(), defined in ec_extra/ec_brainpool.c.
This keeps all Brainpool dispatch logic outside the FIPS boundary and
ensures bcm.o grows by a fixed amount (one call site) rather than
per-curve. Follows the evp_pkey_meth_find + non_fips_pkey_evp_methods
pattern.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Regenerate generated-src for Brainpool test vectors
03bf94f 
Run generate_build_files.py to include the 5 new Wycheproof ECDSA
Brainpool vector files in the pre-generated test data archive used
by no-Go CI toolchains.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Allow Brainpool and secp256k1 curves in EVP TLS encoded point functions
6d0662e 
The EVP_PKEY_{get,set}1_tls_encodedpoint functions had a hardcoded
allowlist of only four NIST curves. The underlying point encoding
(EC_POINT_oct2point, EC_KEY_key2buf) is curve-agnostic, so this
restriction was unnecessarily narrow. Add all remaining built-in
curves (secp256k1 and the five Brainpool curves) to fix strongswan
ECDH interoperability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sfarestam-iproov @claude @justsmth
Add secp256k1 and brainpoolP256r1 to ECTLSEncodedPoint test
f1478b3 
Extend the existing EVP_PKEY_{get,set}1_tls_encodedpoint round-trip
test to cover secp256k1 (Wycheproof vector) and brainpoolP256r1
(RFC 7027 Section A.1), pinning the curve allowlist added in the
previous commit and exercising ECDH derivation for both curves.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@nebeid nebeid nebeid approved these changes
@manastasova manastasova Awaiting requested review from manastasova

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Support for Brainpool Curves (RFC 5639) in TLS 1.2 and TLS 1.3

4 participants

@sfarestam-iproov @codecov-commenter @nebeid @justsmth